-
Notifications
You must be signed in to change notification settings - Fork 148
proxy: download blocks parallely while walking the chain #3892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
3b9ced7 to
e60df25
Compare
596c10c to
23ecf90
Compare
605f08c to
0fc9d5b
Compare
| of "Auto": StdoutLogKind.Auto | ||
| else: StdoutLogKind.None | ||
| maxBlockWalk = jsonNode.getOrDefault("maxBlockWalk").getInt(1000) | ||
| parallelBlockDownloads = jsonNode.getOrDefault("parallelBlockDownloads").getInt(10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this (and all the other jsonNode.getOrDefault("foo").getInt(...) lines), because it's tied to an architecture-size specific integer (int, which is 32-bit on 32-bit platforms and 64-bit on 64-bit platforms), it creates a tricky-to-discover/test failure mode. Instead of using getInt, it's more predictable across 32/64-bit platforms to use getBiggestInt, which returns BiggestInt, currently an int64 regardless of whether it's on a 32-bit or 64-bit platform.
It does say that it's
an alias for the biggest signed integer type the Nim compiler supports. Currently this is
int64, but it is platform-dependent in general.
But at least it's no more, and currently less, platform-dependent than getInt/int.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the consumer of these integers also uses int. Wouldn't it make more sense to be consistent. What is the tricky-to-discover/test failure mode you are talking about? any examples?
EDIT: also why should the bit width of these configuration values be platform independent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's good to be consistent and minimize the integer type conversions required (exception: safe conversions which don't change signedness and don't decrease range, so e.g., uintfoo to uint64 or intfoo to int64). So in general int is a type which might be approached cautiously. Sometimes it's basically required, e.g., anything which ends up as an index into an array or seq directly or indirectly, has to be an int anyway to work. But sometimes there's no strong constraint of that sort and one can get this consistency without using platform-dependent types.
EDIT: also why should the bit width of these configuration values be platform independent?
A couple of main advantages:
-
it means that the end-user UX and functionality is as much the same as possible regardless of whether it's compiled as 32-bit or 64-bit. For the rest of the
nimbus-eth1repo, practically it will almost always be used having been compiled as 64-bit code, but the NVP has a decent chance of being used in some 32-bit applications due to its target embedded functionality/API; and -
arguably at least as importantly, it decreases the scenarios wherein someone tests it on a 32-bit platform and some issue proves difficult to reproduce on the 64-bit platforms which run in CI, on the machines on which we're developing all this software, etc. It's easier for minimal differences to exist which can cause either UX or debugging isues.
| else: | ||
| try: | ||
| downloadedHeaders[nextHash] | ||
| except KeyError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e isn't used here; can just use except KeyError.
| futs = @[] | ||
| downloadedHeaders.clear() | ||
|
|
||
| while nextNum > targetNum and uint64(futs.len) < engine.parallelBlockDownloads: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it won't actually download the targetNum block itself? (> vs >=)
| hash = nextHash, | ||
| number = blk.number, | ||
| remaining = distinctBase(blk.number) - targetNum | ||
| nextNum -= 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/status-im/nimbus-eth1/pull/3892/changes#diff-223d8876a66a7bfacccc6d0d0c52b706341981ee91c954e03c5c4d64a268701fR119 for context, somewhat: because it won't try for targetNum itself this underflow/wraparound won't hit. But if targetNum == 0, and nextNum manages to hit 0, then nextNum -= 1 wraps to high(uint64), and this loop might not exit, because nextNum > targetNum will be true again.
No description provided.